fix(datetime-button): fix initial text not obeying datetime constraints#31218
fix(datetime-button): fix initial text not obeying datetime constraints#31218droc101 wants to merge 6 commits into
Conversation
…aints expose new `getClosestDate(date: Date) => Promise<Date>` function in the Datetime class and use it in DatetimeButton to round the current time (default value) into a date that matches the constraints provided on the Datetime element (dayValues, minuteValues, etc.) closes ionic-team#30183
|
@droc101 is attempting to deploy a commit to the Ionic Team on Vercel. A member of the Team first needs to authorize it. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
thetaPC
left a comment
There was a problem hiding this comment.
Please also add tests to prevent any regressions in the future. I would recommend adding them to datetime-button/test/basic/datetime-button.e2e.ts:
presentation="date"with no constraints — guards the plain day/month fallback (this is the case the current bug breaks).presentation="time"withminute-values="0"— the originally reported case from #30183.- At least one other constraint, e.g.
hour-valuesormonth-values, to confirm it's not minute-specific.
| month: date.getMonth(), | ||
| day: date.getDay(), |
There was a problem hiding this comment.
DatetimeParts and the native Date API don't use the same conventions, and two fields are mismatched here:
-
monthis off by one.Date.getMonth()is 0-indexed (January is0), butDatetimeParts.monthis 1-indexed (January is1). So today, June, comes through as month 5, whichDatetimePartsreads as May. -
dayis the wrong field entirely.Date.getDay()returns the day of the week (0–6, Sun–Sat), not the day of the month. The day of the month comes fromDate.getDate().
You can reproduce it with a plain no-value date picker (no constraints needed): the calendar lands on Jun 17, 2026 but the button reads "May 3, 2026".
Both of these can be seen with:
<ion-item>
<ion-label>Start Date</ion-label>
<ion-datetime-button slot="end" datetime="no-value-date"></ion-datetime-button>
</ion-item>
<!-- keep-contents-mounted makes the datetime (and the button text) compute on load, so the mismatch shows immediately -->
<ion-modal keep-contents-mounted="true">
<ion-datetime locale="en-US" presentation="date" id="no-value-date"></ion-datetime>
</ion-modal>Fix:
| month: date.getMonth(), | |
| day: date.getDay(), | |
| month: date.getMonth() + 1, | |
| day: date.getDate(), |
| const defaultDatetime = [(await datetimeEl.getClosestDate(new Date())).toISOString()]; | ||
| const parsedDatetimes = parseDate(parsedValues.length > 0 ? parsedValues : defaultDatetime); |
There was a problem hiding this comment.
getClosestDate is awaited on every call, but the result is thrown away whenever parsedValues is non-empty (i.e. whenever the datetime has a value). Since getClosestDate is a Stencil @Method, this is an async cross-component round-trip on every ionValueChange, just to discard the result. Move it into the no-value branch and also update the comment:
| const defaultDatetime = [(await datetimeEl.getClosestDate(new Date())).toISOString()]; | |
| const parsedDatetimes = parseDate(parsedValues.length > 0 ? parsedValues : defaultDatetime); | |
| /** | |
| * Both ion-datetime and ion-datetime-button default to today's date | |
| * and time if no value is set. The datetime is queried for the closest | |
| * valid value so the button respects the same constraints (min, max, | |
| * minuteValues, etc.) that the datetime applies to its own default. | |
| */ | |
| let valuesToParse = parsedValues; | |
| if (valuesToParse.length === 0) { | |
| const closestDate = await datetimeEl.getClosestDate(new Date()); | |
| valuesToParse = [closestDate.toISOString()]; | |
| } | |
| const parsedDatetimes = parseDate(valuesToParse); |
| /** | ||
| * Get the closest valid Date according to the restrictions on this Datetime | ||
| * @param date The Date to find the closest valid value for | ||
| */ | ||
| @Method() | ||
| async getClosestDate(date: Date) { | ||
| const closest = this.getClosestDatetimeParts({ | ||
| month: date.getMonth(), | ||
| day: date.getDay(), | ||
| year: date.getFullYear(), | ||
| dayOfWeek: date.getDay(), | ||
| hour: date.getHours(), | ||
| minute: date.getMinutes(), | ||
| }); | ||
| return removeDateTzOffset(new Date(convertDataToISO(closest))); | ||
| } |
There was a problem hiding this comment.
I'd recommend an @internal method instead of a new public one.
componentWillLoad already computes this.defaultParts (today snapped to the closest valid value via getClosestValidDate, datetime.tsx:1540) — exactly what the button is trying to reconstruct. So the button can just read that value rather than recomputing it from a Date, which also guarantees the button and picker never disagree.
Making it @internal follows how these two already communicate (the button listens to the @internal ionValueChange event), and avoids adding public API to document and support. It also avoids reconstructing parts from a Date (the ISO -> removeDateTzOffset round-trip), which is where the getMonth()/getDay() mistakes lived.
| /** | |
| * Get the closest valid Date according to the restrictions on this Datetime | |
| * @param date The Date to find the closest valid value for | |
| */ | |
| @Method() | |
| async getClosestDate(date: Date) { | |
| const closest = this.getClosestDatetimeParts({ | |
| month: date.getMonth(), | |
| day: date.getDay(), | |
| year: date.getFullYear(), | |
| dayOfWeek: date.getDay(), | |
| hour: date.getHours(), | |
| minute: date.getMinutes(), | |
| }); | |
| return removeDateTzOffset(new Date(convertDataToISO(closest))); | |
| } | |
| /** | |
| * Returns the default parts the datetime falls back to when no value is set: | |
| * today's date and time snapped to the closest value allowed by the | |
| * component's constraints (`min`, `max`, and the `*Values` props). | |
| * | |
| * @internal | |
| */ | |
| @Method() | |
| async getDefaultPart(): Promise<DatetimeParts> { | |
| return this.defaultParts; | |
| } |
The datetime-button.tsx then reads it in the no-value branch
/**
* Both ion-datetime and ion-datetime-button default to today's date and
* time if no value is set. We read the datetime's computed default so the
* button respects the same constraints (min, max, minuteValues, etc.) that
* the datetime applies to its own fallback, instead of using a raw "now".
*/
const parsedDatetimes =
parsedValues.length > 0 ? parseDate(parsedValues) : [await datetimeEl.getDefaultPart()];…method returning datetime's defaultParts
|
Tests will be coming in a separate future commit, have to figure out how that side of the code works. |
|
@droc101 I highly recommend reviewing the E2E testing doc. |
- should default to exact current time with no constraints - should obey minuteValues constraint - should obey hourValues constraint - should obey monthValues constraint
| * Get the closest valid DatetimeParts according to the restrictions on this Datetime | ||
| * @param parts The DatetimeParts to find the closest valid value for | ||
| */ | ||
| private getClosestDatetimeParts(parts: DatetimeParts) { |
There was a problem hiding this comment.
This helper can go away. It only exists because the earlier getClosestDate(date) method needed reusable logic, but now that getDefaultPart() just returns the already-computed this.defaultParts, this has a single caller (componentWillLoad). I'd revert it back to how it was originally.
| const parsedDatetimes = parseDate( | ||
| parsedValues.length > 0 ? parsedValues : [convertDataToISO(await datetimeEl.getDefaultPart())] | ||
| ); |
There was a problem hiding this comment.
getDefaultPart() already returns DatetimeParts, so wrapping it in convertDataToISO(...) only to feed it back through parseDate is a parts → ISO → parts round-trip. You can read it directly and skip both the conversion and the convertDataToISO import:
| const parsedDatetimes = parseDate( | |
| parsedValues.length > 0 ? parsedValues : [convertDataToISO(await datetimeEl.getDefaultPart())] | |
| ); | |
| const parsedDatetimes = | |
| parsedValues.length > 0 ? parseDate(parsedValues) : [await datetimeEl.getDefaultPart()]; |
Unless there's a reason of why you went with this approach?
| } | ||
|
|
||
| componentOnReady(datetimeEl, () => { | ||
| componentOnReady(datetimeEl, async () => { |
There was a problem hiding this comment.
Why is this change necessary?
| * text in the buttons. | ||
| */ | ||
| this.setDateTimeText(); | ||
| await this.setDateTimeText(); |
There was a problem hiding this comment.
Why is this change necessary?
| * to the locale specified on ion-datetime. | ||
| */ | ||
| private setDateTimeText = () => { | ||
| private setDateTimeText = async () => { |
There was a problem hiding this comment.
Why is this change necessary?
| this.processMinParts(); | ||
| this.processMaxParts(); | ||
|
|
||
| this.defaultParts = getClosestValidDate({ |
There was a problem hiding this comment.
As mentioned in https://github.com/ionic-team/ionic-framework/pull/31218/changes#r3463042864, I would revert this.
| ); | ||
| await page.locator('.datetime-ready').waitFor(); | ||
|
|
||
| await page.pause(); |
There was a problem hiding this comment.
Please remove the await page.pause() calls from these tests. page.pause() is a debugging only tool that halts the run and opens the Playwright Inspector, so it shouldn't ship in the suite.
| }); | ||
|
|
||
| test.describe(title('datetime-button: datetime constraints'), () => { | ||
| test('should default to exact current time with no constraints', async ({ page }) => { |
There was a problem hiding this comment.
These constraint tests should be annotated with the issue they're covering, like the existing #27797 tests in this file do.
|
|
||
| await page.pause(); | ||
|
|
||
| const expectedTime = fixedTime; |
There was a problem hiding this comment.
expectedTime = fixedTime copies the reference, so setMinutes/setHours/setMonth mutate fixedTime itself. It's harmless here only because fixedTime isn't read again.
| const expectedTime = fixedTime; | |
| const expectedTime = new Date(fixedTime); |
| }); | ||
| }); | ||
|
|
||
| test.describe(title('datetime-button: datetime constraints'), () => { |
There was a problem hiding this comment.
Optional / nice-to-have, take it or leave it. These four tests repeat the same clock setup and formatter definitions. We already share per-describe setup with beforeEach elsewhere in this file (see datetime-button.e2e.ts:27 in the "switching to correct view" describe), so we could do the same here. Hoist the constant time and the formatters to the describe scope and set the clock in a beforeEach, then each test only carries its own setContent and expected value:
test.describe(title('datetime-button: datetime constraints'), () => {
const fixedTime = new Date('2026-06-18T17:54:54.518Z');
const dateFormat = new Intl.DateTimeFormat('en-US', {
weekday: 'short',
month: 'long',
day: '2-digit',
});
const timeFormat = new Intl.DateTimeFormat('en-US', {
hour: '2-digit',
minute: '2-digit',
});
test.beforeEach(async ({ page }) => {
await page.clock.setFixedTime(fixedTime);
});Then each test drops its local fixedTime, setFixedTime, and Intl.DateTimeFormat lines and just uses the shared ones. For the expected value, clone before mutating so the shared fixedTime isn't altered between tests:
const expectedTime = new Date(fixedTime);
expectedTime.setMinutes(0);.One caveat if you do take this: once fixedTime is shared across tests, cloning it before mutating becomes necessary (not just tidy), otherwise a mutation in one test leaks into the others via beforeEach.
|
Small nit on the PR title: the |
Issue number: resolves #30183
What is the current behavior?
an
IonDatetimeButton's default value (such as when .value is undefined) always uses the exact current date & time, regardless of any restrictions requested by the associatedIonDatetime(such as only allowing 5 minute increments)What is the new behavior?
IonDatetimeButtonnow follows the constraints on its associatedIonDatetime.valueof theIonDatetimeButtonis unchangedasync getClosestDate(date: Date) => Promise<Date>has been added toIonDatetimeto facilitate this change. If this should not be a public method, I can work on changing that.getClosestDatetimeParts(parts: DatetimeParts) => DatetimePartshas been added toIonDatetimeas a helper to the previously mentionedgetClosestDatemethod, and to deduplicate code.Does this introduce a breaking change?